Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expression evaluator decides module call values using instances.Expander and namedvals.State #34545

Merged
merged 4 commits into from
Jan 19, 2024

Conversation

apparentlymart
Copy link
Contributor

We've been gradually working to factor out fiddly details of how we do instance expansion and how we handle named values over many previous PRs over the last few years, so that we can build Terraform's features more in terms of well-tested internal abstractions rather than sprawled inline business logic.

Now we'll make use of that effort to simplify how the modules runtime's expression evaluator decides the value to use when referring to a module call, using a reference like module.foo. Whereas before this had to deal with various fiddly cases such as values in the plan overriding values in the state, we can now rely entirely on our instances.Expander and namedvals.State APIs to simplify the inline logic considerably.

My goal here was to have this logic focused primarily on the mechanics of translating the data into the cty.Value form that the caller needs, with the worries about exactly how the data got there factored out. The existing special case of returning some differently-shaped results during the validate walk meant that I didn't quite reach that ideal, but I think this is close enough for now.

If I've succeeded in my goal here, this should not change the externally-observable behavior at all. (That largely relies on the correctness of several earlier PRs that arranged for the instances.Expander and namedvals.State to get populated with suitable data.)


Because the instances.Expander API already has some modelling of unknown count or for_each expressions in module calls, this also represents a very small step in the direction of closing #30937 -- returning a cty.DynamicVal placeholder for any module call that has unknown expansion -- although that situation isn't really reachable in practice yet because there is not yet any caller that actually registers a module call as having an unknown expansion. More work in that direction will follow in future PRs.


The unit test TestEvaluatorGetModule previously tried a bunch of different variations of different ways to get data into the evaluator's GetModule function because of all of the tricky inline logic, but now that we've moved the responsibility for making those decisions elsewhere (in earlier PRs) the test can be considerably simpler: just arranging for the instances.Expander and namedvals.State to be populated appropriately.

This test could potentially be expanded in future to cover more variations such as simulating a module call with count or for_each, but we've previously handled those situations primarily through this package's integration tests and so there's already plentiful test coverage, and so I can't justify spending lots of time writing redundant tests there today.

This new method calculates the static Module address corresponding to the
receiver, effectively discarding any instance keys present in the
containing module instance path.
This is just a variation of the pre-existing ModuleInstance.Call that
returns its result as a single value of type AbsModuleCall.

This should really have been the signature of ModuleInstance.Call in the
first place, but it took us some time to realize that we would benefit
from an absolute address type for module calls themselves as opposed to
the instances they imply once expanded, and so AbsModuleCall didn't
exist at the time we originally wrote ModuleInstance.Call.

Perhaps one day we'll update all existing callers of ModuleInstance.Call
to use ModuleInstance.AbsCall instead, but for now we'll be pragmatic and
keep both to avoid causing unnecessary churn.
This is a similar idea to Expander.ExpandModule but for situations where
we already know exactly which containing module we're evaluating inside,
and thus we can just pluck out the leaf instance keys of that specific
dynamic call, rather than the full recursive expansion of every containing
module.

Expander.ExpandModule is useful when implementing DynamicExpand for a
graph node because main graph nodes always represent static configuration
objects, but this new Expander.ExpandAbsModuleCall is more appropriate
for use during expression evaluation of references to a module call
because in that case we'll know which specific module instance address
we're using as the evaluation scope.

A future commit will actually use this new method from the expression
evaluator in the modules runtime.
We've been gradually improving the design of how we keep track of "named
values" such as output values, but the expression evaluator for output
values was still doing some things in more manual/bespoke way, rather
than making use of the new abstractions.

We'll now use a combination of instances.Expander and namedvalues.State
as the source of truth for which instances exist for a given module call
and what output values each of those instances have.

The configuration remains the decider for which output value names are
declared, but the final result associated with each of the declared output
values is tracked by namedvalues.State.

This also removes the temporary namedvals GetOutputValuesForModuleCall
method we added as a stopgap to allow introducing the namedvals.State
API without such a disruptive rewrite of our output value handling.
We have no further need for that helper because instances.Expander already
knows which instances should exist for each module call, and the
configuration knows which output values should exist in the result, and
so namedvals only needs to be able to return one final output value result
at a time, via namedvals.State.GetOutputValue.

With all of the tricky inline logic factored out of this part of the
evaluator, the TestEvaluatorGetModule test becomes considerably simpler
since it's no longer the evaluator's responsibility to choose between
various different sources for the module output values depending on the
situation, and instead we just need to set the evaluator and the instance
expander up in a realistic way, with the correct handling of both of those
utilities already being covered by this package's main integration tests.
@apparentlymart apparentlymart merged commit 9851cd4 into main Jan 19, 2024
6 checks passed
@apparentlymart apparentlymart deleted the f-modules-eval-outputs-from-config branch January 19, 2024 21:52
Copy link

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants